otel-thread-ctx: address review feedback from #347#366
Merged
Conversation
Four independently-flagged concerns: - Accept optional third `attributes` arg in ThreadContext constructor. The TS ThreadContextCtor.new type declares `attributes` optional, but the C++ side hard-errored on `args.Length() != 3`. Loosen to accept 2 or 3 args; EncodeAttrs already handled undefined/null attrs_val correctly. - Fold `cleanup_registered` into `undefined_addr`. Uses the field's zero / non-zero state as the 'already-registered' flag: it starts at zero, ResetDiscoveryStruct clears it back to zero (so a re-init on the same thread would re-register), and any real V8 undefined singleton address is non-zero. Removes the separate thread_local static. - Coerce attribute values to strings in a pre-pass in EncodeAttrs before writing to the output buffer. Value->ToString may execute user JS (custom toString methods) which could re-enter into the ThreadContext via appendAttributes and interleave with our writes. Separating the coerce phase from the encode phase keeps the encode phase re-entrancy-free. - Make the 'enterWithContext attaches the record to the current async scope' test callback `async` and `await tcRun(...)`. Previously the callback returned a promise via `void tcRun(...)` and the promise's inner `.then` assertion could fire an unhandled rejection instead of a test failure.
Overall package sizeSelf size: 2.41 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | pprof-format | 2.2.2 | 500.53 kB | 500.53 kB | | source-map | 0.7.6 | 185.63 kB | 185.63 kB | | node-gyp-build | 4.8.4 | 13.86 kB | 13.86 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
szegedi
added a commit
to szegedi/custom-labels
that referenced
this pull request
Jul 3, 2026
Four independently-flagged concerns, ported from the equivalent fixes on the pprof-nodejs vendored copy (DataDog/pprof-nodejs#366): - Accept optional third `attributes` arg in ThreadContext constructor. The TS ThreadContextCtor.new type declares `attributes` optional, but the C++ side hard-errored on `args.Length() != 3`. Loosen to accept 2 or 3 args; EncodeAttrs already handled undefined/null attrs_val correctly. - Fold `cleanup_registered` into `undefined_addr`. Uses the field's zero / non-zero state as the 'already-registered' flag: it starts at zero, ResetDiscoveryStruct clears it back to zero (so a re-init on the same thread would re-register), and any real V8 undefined singleton address is non-zero. Removes the separate thread_local static. - Coerce attribute values to strings in a pre-pass in EncodeAttrs before writing to the output buffer. Value->ToString may execute user JS (custom toString methods) which could re-enter into the ThreadContext via appendAttributes and interleave with our writes. Separating the coerce phase from the encode phase keeps the encode phase re-entrancy-free. - Make the 'enterWithContext attaches the record to the current async scope' test callback `async` and `await tcRun(...)`. Previously the callback returned a promise via `tcRun(...)` and the promise's inner `.then` assertion could fire an unhandled rejection instead of a test failure.
3 tasks
nsavoire
reviewed
Jul 13, 2026
| // re-enter into this ThreadContext (e.g. via `appendAttributes`) and | ||
| // interleave with our writes to `out`. Doing all such coercion BEFORE | ||
| // touching `out` keeps the encode phase re-entrancy-free. | ||
| std::vector<std::pair<uint32_t, Local<String>>> coerced; |
There was a problem hiding this comment.
I don't think this fixes the re-entrancy, out was already a local variable in Append, and therefore not shared across re-entrant calls.
The issue is current_used in Append:
const size_t current_used = self->record_->attrs_data_size; // (A) snapshot taken
std::vector<uint8_t> appended;
bool truncated = false;
if (!EncodeAttrs(isolate, context, args[0], current_used, &appended, &truncated)) { // (B) can re-enter
return;
}
...
const size_t new_used = current_used + appended.size(); // (C) uses the stale snapshot
Also current_used is passed as existing_size to EncodeAttrs and might also be stale in case of re-rentrancy.
Author
There was a problem hiding this comment.
fair enough, I'll add a reentrancy guard as you originally suggested :-)
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05eec43789
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| Local<String> v; | ||
| if (!val_val->ToString(context).ToLocal(&v)) return false; | ||
| coerced.emplace_back(i, v); |
There was a problem hiding this comment.
When appendAttributes receives an object whose toString calls appendAttributes on the same context, this pre-pass executes that JS before the outer Append mutates the record, but Append has already captured current_used and passes it as existing_size before calling EncodeAttrs. After the inner append grows or advances attrs_data_size, the outer append still writes at the old offset and stores old + outer size, so the inner attributes can be overwritten or dropped; refresh the record size/capacity after coercion or guard against re-entrant append before encoding/appending.
Useful? React with 👍 / 👎.
Also undo the two-phase encoding loop that was the previous (insufficient) attempt at fixing the reentrancy.
This comment has been minimized.
This comment has been minimized.
Author
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
nsavoire
approved these changes
Jul 13, 2026
Merged
szegedi
added a commit
that referenced
this pull request
Jul 14, 2026
* Address review feedback from PR #347 Four independently-flagged concerns: - Accept optional third `attributes` arg in ThreadContext constructor. The TS ThreadContextCtor.new type declares `attributes` optional, but the C++ side hard-errored on `args.Length() != 3`. Loosen to accept 2 or 3 args; EncodeAttrs already handled undefined/null attrs_val correctly. - Fold `cleanup_registered` into `undefined_addr`. Uses the field's zero / non-zero state as the 'already-registered' flag: it starts at zero, ResetDiscoveryStruct clears it back to zero (so a re-init on the same thread would re-register), and any real V8 undefined singleton address is non-zero. Removes the separate thread_local static. - Coerce attribute values to strings in a pre-pass in EncodeAttrs before writing to the output buffer. Value->ToString may execute user JS (custom toString methods) which could re-enter into the ThreadContext via appendAttributes and interleave with our writes. Separating the coerce phase from the encode phase keeps the encode phase re-entrancy-free. - Make the 'enterWithContext attaches the record to the current async scope' test callback `async` and `await tcRun(...)`. Previously the callback returned a promise via `void tcRun(...)` and the promise's inner `.then` assertion could fire an unhandled rejection instead of a test failure. * Explicitly guard against reentrant Append calls. Also undo the two-phase encoding loop that was the previous (insufficient) attempt at fixing the reentrancy.
szegedi
added a commit
that referenced
this pull request
Jul 15, 2026
* Address review feedback from PR #347 Four independently-flagged concerns: - Accept optional third `attributes` arg in ThreadContext constructor. The TS ThreadContextCtor.new type declares `attributes` optional, but the C++ side hard-errored on `args.Length() != 3`. Loosen to accept 2 or 3 args; EncodeAttrs already handled undefined/null attrs_val correctly. - Fold `cleanup_registered` into `undefined_addr`. Uses the field's zero / non-zero state as the 'already-registered' flag: it starts at zero, ResetDiscoveryStruct clears it back to zero (so a re-init on the same thread would re-register), and any real V8 undefined singleton address is non-zero. Removes the separate thread_local static. - Coerce attribute values to strings in a pre-pass in EncodeAttrs before writing to the output buffer. Value->ToString may execute user JS (custom toString methods) which could re-enter into the ThreadContext via appendAttributes and interleave with our writes. Separating the coerce phase from the encode phase keeps the encode phase re-entrancy-free. - Make the 'enterWithContext attaches the record to the current async scope' test callback `async` and `await tcRun(...)`. Previously the callback returned a promise via `void tcRun(...)` and the promise's inner `.then` assertion could fire an unhandled rejection instead of a test failure. * Explicitly guard against reentrant Append calls. Also undo the two-phase encoding loop that was the previous (insufficient) attempt at fixing the reentrancy.
szegedi
added a commit
that referenced
this pull request
Jul 15, 2026
* Address review feedback from PR #347 Four independently-flagged concerns: - Accept optional third `attributes` arg in ThreadContext constructor. The TS ThreadContextCtor.new type declares `attributes` optional, but the C++ side hard-errored on `args.Length() != 3`. Loosen to accept 2 or 3 args; EncodeAttrs already handled undefined/null attrs_val correctly. - Fold `cleanup_registered` into `undefined_addr`. Uses the field's zero / non-zero state as the 'already-registered' flag: it starts at zero, ResetDiscoveryStruct clears it back to zero (so a re-init on the same thread would re-register), and any real V8 undefined singleton address is non-zero. Removes the separate thread_local static. - Coerce attribute values to strings in a pre-pass in EncodeAttrs before writing to the output buffer. Value->ToString may execute user JS (custom toString methods) which could re-enter into the ThreadContext via appendAttributes and interleave with our writes. Separating the coerce phase from the encode phase keeps the encode phase re-entrancy-free. - Make the 'enterWithContext attaches the record to the current async scope' test callback `async` and `await tcRun(...)`. Previously the callback returned a promise via `void tcRun(...)` and the promise's inner `.then` assertion could fire an unhandled rejection instead of a test failure. * Explicitly guard against reentrant Append calls. Also undo the two-phase encoding loop that was the previous (insufficient) attempt at fixing the reentrancy.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to review comments on #347 that landed after merge. Four independent, non-API-surface fixes.
Changes
Accept optional third
attributesarg inThreadContextconstructor. The TSThreadContextCtor.newtype declaresattributesoptional, but the C++ side hard-errored onargs.Length() != 3. Loosen to< 2 || > 3;EncodeAttrsalready handledundefined/nullcorrectly. (review comment)Fold
cleanup_registeredintoundefined_addr. Uses the field's zero / non-zero state as the "already-registered" flag: it starts at zero (thread-local zero-init),ResetDiscoveryStructclears it back to zero (so a re-init on the same thread would re-register), and any real V8 undefined-singleton address is non-zero. Removes the separatethread_local bool.Coerce attribute values to strings in a pre-pass in
EncodeAttrs, before touching the output buffer.Value->ToStringmay execute user JS (customtoString) which could re-enter into the sameThreadContextviaappendAttributesand interleave with our writes. Separating the coerce phase from the encode phase keeps the encode phase re-entrancy-free.Make the "enterWithContext attaches the record to the current async scope" test callback
asyncandawait tcRun(...). Previously the callback returned a promise viavoid tcRun(...)and the promise's inner.thenassertion could fire as an unhandled rejection instead of a proper test failure.Test plan
npm run test:docker— 150/150 passing (unchanged)toStringthat callsappendAttributes), but the two-phase encode already prevents interleaving; leaving that as a possible follow-up.Related
These same concerns likely apply to the upstream vendored copy in polarsignals/custom-labels (PR #17). I'll port them there next.
Jira: PROF-14694